Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding reactive html export #1360

Merged
merged 9 commits into from
May 19, 2024

Conversation

gvarnavi
Copy link
Contributor

Big fan of the latest islands feature!

This PR extends the _server and _cli export functionality to return an interactive HTML using marimo islands.

CLI usage (note CLI args available in regular html export are currently ignored):

marimo export html notebook.py --reactive --no-include-code -o notebook-reactive.html

Low-level python usage (e.g. what I use to define ObservableHQ data-loaders):

from marimo._server.export import run_app_then_export_as_reactive_html
from marimo._utils.marimo_path import MarimoPath
from bs4 import BeautifulSoup
import asyncio

path = MarimoPath("src/data-files/notebook.py")
html, _ = asyncio.run(
    run_app_then_export_as_reactive_html(
        path, include_code=False
    )
)

soup = BeautifulSoup(html, features="html.parser")
body = soup.body.decode_contents().strip()
print(body)

Happy to include CLI tests if this looks good!

Copy link

vercel bot commented May 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2024 4:09pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2024 4:09pm

Copy link

github-actions bot commented May 11, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@gvarnavi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA.

@gvarnavi
Copy link
Contributor Author

recheck

@mscolnick
Copy link
Contributor

I like this - we should probably expose a low-level python api that isn't private (don't want to break APIs) and hopefully the smallest api (just a path?)

@mscolnick
Copy link
Contributor

or maybe we don't need the low-level api? would you still use it, if this CLI addition was added?

@gvarnavi
Copy link
Contributor Author

I would probably still call this from the low-level api since I suspect most SSGs (e.g. Observable Framework) won't handle the html as is, and one might as-well manipulate it further with python (e.g. using BeautifulSoup as above).

Copy link
Contributor

@akshayka akshayka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool! We should definitely have a way to export WASM-powered HTML. Just yesterday someone asked me for this.

A few comments.

  1. I tried using the CLI export on the intro.py tutorial and noticed that some styles were off (notebook width, codemirror editors, and KaTeX didn't render are a few things I noticed). marimo export should look good out of the box.

  2. If we add a CLI, we could omit the low-level API. When integrating with Observable, you could just run the marimo CLI (either in a Python script using subprocess, or in a shell script, etc), unless I am missing something.

  3. It seems odd that cli-args is ignored when --reactive is passed. That sort of suggests to me that this should be its own export option, eg marimo export wasm or marimo export reactive-html.

If we are not prepared to do the work required for (1) yet, then we could opt for a small low-level API and revisit the CLI later ...

marimo/_cli/export/commands.py Outdated Show resolved Hide resolved
@gvarnavi
Copy link
Contributor Author

Thanks for the prompt response!

  1. I tried using the CLI export on the intro.py tutorial and noticed that some styles were off (notebook width, codemirror editors, and KaTeX didn't render are a few things I noticed). marimo export should look good out of the box.

This uses MarimoIslandStub.render() directly - but agreed we should perhaps look at the tweaking some inline css styles or similar. FWIW, when I use this to embed on Observable KaTeX seems to work?

  1. If we add a CLI, we could omit the low-level API. When integrating with Observable, you could just run the marimo CLI (either in a Python script using subprocess, or in a shell script, etc), unless I am missing something.

Sure yeah - to clarify, I'm perfectly happy calling the low-level "private" API as above (which presumably the CLI will call as-well) or, like you said, running the CLI tool and then piping the output to a different process for post-processing.

  1. It seems odd that cli-args is ignored when --reactive is passed. That sort of suggests to me that this should be its own export option, eg marimo export wasm or marimo export reactive-html.

marimo export reactive-html was my initial thought, and then I realized I was essentially copying a lot of the html code and switched to this. Happy to go back if you think it's cleaner.

Copy link
Collaborator

@dmadisetti dmadisetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! So Observable data loaders use require a *.html.py* right? Or since invoking from marimo, are you using a *.html.sh?

marimo/_server/export/__init__.py Outdated Show resolved Hide resolved
marimo/_server/export/__init__.py Outdated Show resolved Hide resolved
@gvarnavi
Copy link
Contributor Author

Cool! So Observable data loaders use require a *.html.py* right? Or since invoking from marimo, are you using a *.html.sh?

@dmadisetti Indeed, the simplest case would be a data-loader called marimo-intro-cli.html.sh with the contents

#!/bin/bash
marimo export html src/data-files/intro-notebook.py --reactive --no-include-code

which one can call as follow in an Observable Framework md file:

```js
const marimo_html = FileAttachment("data/marimo-intro-cli.html").html();
```

<div style="max-width:740px; margin: 0 auto;">
  <div id="intro-notebook"> ${marimo_html.body} </div>
</div>

Added the repo on git, if you wanted to have a look. Deployed site can be found here.

@dmadisetti
Copy link
Collaborator

If the data loader just goes off the shebang, I think you should be able to do

#!/user/bin/env marimo  export html --reactive --no-include-code

import marimo as mo
# normal app here
# ...

@mscolnick
Copy link
Contributor

@gvarnavi, given @dmadisetti's comment of using the shabang, maybe we could just do this? this would require no changes to the library and gives you a lot of flexibility in the output (b/c im sure the default html export from the CLI is not perfect)

#!/user/bin/env python

from marimo import MarimoIslandGenerator

  generator = MarimoIslandGenerator()
  block1 = generator.add_code("import marimo as mo")
  block2 = generator.add_code("mo.md('Hello, islands!')")

  # Build the app
  app = await generator.build()

  # Render the app
  output = f"""
  <html>
      <head>
          {generator.render_head()}
      </head>
      <body>
          {block1.render(display_output=False)}
          {block2.render()}
      </body>
  </html>
  """
... 

if it's because you are coming from a python file instead of code snippets, we can add a class static method

  generator = MarimoIslandGenerator.from_file("path/to/file.py")
  # same code

@gvarnavi
Copy link
Contributor Author

To clarify, I don't necessarily need this PR for my purposes - I'm perfectly happy using the snippet I added in export.__init__.py as my Observable dataloader. This was mostly because I wanted to contribute to the codebase and figured others might also like a reactive HTML export!

Pushed the changes I was working on before your comment @mscolnick - I think it might capture atleast some of @akshayka concerns, if you want to check it out.

Regardless of the cli, a static MarimoIslandGenerator.from_file() does sound like a good idea.

@dmadisetti
Copy link
Collaborator

I think this makes sense; islands are a little bit hidden otherwise. Regarding the limited format, I think this and normal export HTML - could be expanded to take a template, but that probably doesn't need to be added until it's needed.

@akshayka
Copy link
Contributor

@gvarnavi , thanks for clarifying, and thanks for contributing back to us! We definitely appreciate it :)

@dmadisetti, ok, good point -- wasm export is hard to find/low-level with just islands.

If I understood the above discussion, it will be sufficient to just add reactive HTML export to the CLI; that will enable both gvarnavi's use case as well as let less advanced users benefit from WASM exports. If this is correct, then I support adding reactive HTML export to the CLI.

@gvarnavi, I took a look and the styling does look better -- thank you. Three comments:

  1. Margins. Below I've pasted a side-by-side comparison of marimo run intro.py (left) and marimo export html --reactive --no-include-code intro.py (right). Notice that the right-hand export is missing vertical margins. Is this possible to fix?

image

  1. Reactivity. When testing locally, the exported HTML is not actually reactive. Is this just a me problem? Here's what I see in the console:
Not allowed to load local resource: blob:null/049a4c1b-06df-4771-a0fa-50275133aa6b
  1. CLI Args. Why not support CLI args? If it's too hard to support, we should at least raise an Exception if cli_args are non-empty when exporting reactive HTML.

@dmadisetti
Copy link
Collaborator

Oh yikes. # 3 might be from a work around I did to get a local cross site Web Worker. Never tested from a non-served file. This might work in islands bridge: https://gist.github.com/walfie/a80c4432bcff70fb826d5d28158e9cc4

But will probably need to tinkering. I know Myles has taken a pass at that web worker too:

const js = `import ${JSON.stringify(new URL(url, import.meta.url))}`;

@mscolnick
Copy link
Contributor

@dmadisetti @akshayka , im pretty sure you can't run a webworker from file:// (or at least ours) - needs to be a hosted server

@akshayka
Copy link
Contributor

(cc @gvarnavi)

im pretty sure you can't run a webworker from file:// (or at least ours) - needs to be a hosted server

Oh shoot -- I wasn't aware of that. I guess that's for security?

If that's the case we might as well not include this in the CLI, and just recommend that people use marimo.io or islands -- can already imagine all the questions we'd get from our users about why their reactive exports aren't working.

@mscolnick I'll leave the decision up to you on how to proceed.

@gvarnavi
Copy link
Contributor Author

im pretty sure you can't run a webworker from file:// (or at least ours) - needs to be a hosted server

Ah indeed, hadn't realized either since I was mostly testing directly on Observable - which is hosted.

Hmm, perhaps the easiest solution then would be a static MarimoIslandGenerator.from_file() class method, and some additional documentation/examples pointing users to it, as @mscolnick suggests -- I can add that sometime tomorrow.

@mscolnick
Copy link
Contributor

@gvarnavi that sounds great. i think docs could be improved for sure - i would appreciate your help, or if you have ideas, i can also incorporate them. i think snippets for users to copy to integrate into their framework of choice (e.g. Observable) go a long way since its easy to modify/adapt without creating too many permutations of options

@dmadisetti
Copy link
Collaborator

dmadisetti commented May 15, 2024

If that's the case we might as well not include this in the CLI, and just recommend that people use marimo.io or islands -- can already imagine all the questions we'd get from our users about why their reactive exports aren't working.

If the major concern is communication to users, then I think this could be mitigated with a alert dialog with something like:

Caution

marimo requires Web Workers to run reactively, and as such marimo islands will not work with local files. Please host this file.

Because not allowing export doesn't get rid of the problem that islands won't work locally. But addressing with a notification enables this export, and documents the edgecase

@gvarnavi
Copy link
Contributor Author

Pardon the delay, just got round to this.

Added a static MarimoIslandGenerator.from_file() method per @mscolnick suggestion. This indeed simplifies the islands html generation a fair bit. e.g. I use the following snippet for Observable.

import asyncio

from marimo import MarimoIslandGenerator

generator = MarimoIslandGenerator.from_file("src/data-files/dislocation-fields.py")
app = asyncio.run(generator.build())
body = generator.render_body()
print(body)

Note: the new render_body() and render_html() functions are nice-to-have but not necessary utility functions.

Also, I've kept the cli export for now (and switched it to using the simpler static method above), until we reach a consensus on whether or not to include it. I like @dmadisetti's warning idea -- perhaps this should be implemented at the islands js file though? As in check if the file is served from a local file (perhaps using window.location.protocol or similar), and issue a warning if so?

Copy link
Contributor

@mscolnick mscolnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! i think if we can remove exposing the CLI --reactive/--no-reactive for now, until i can add the warning. the other additions are great and easy enough to make the CLI

with;

    import os

    from marimo._islands.island_generator import MarimoIslandGenerator

    generator = MarimoIslandGenerator.from_file(
        path.absolute_name, display_code=include_code
    )
    await generator.build()
    html = generator.render_html()

marimo/_islands/island_generator.py Show resolved Hide resolved
marimo/_islands/island_generator.py Outdated Show resolved Hide resolved
marimo/_cli/export/commands.py Outdated Show resolved Hide resolved
mscolnick
mscolnick previously approved these changes May 18, 2024
@gvarnavi
Copy link
Contributor Author

Great, thanks for the review @mscolnick!

@mscolnick
Copy link
Contributor

force merging - looks like unrelated tests are failing on main

@mscolnick mscolnick merged commit ba2529a into marimo-team:main May 19, 2024
11 of 26 checks passed
Copy link

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.6.1-dev6

Haleshot added a commit to Haleshot/marimo that referenced this pull request May 20, 2024
mscolnick added a commit that referenced this pull request May 20, 2024
* adding reactive_html export

* doctype to avoid quirks mode

* respect config width

* clarified help message

* islands from_file static method

* switching cli export to static method

* adding ; between styles

* removing exposing cli option

---------

Co-authored-by: Myles Scolnick <myles@marimo.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants